Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DedupingBucketRateLimiter #6025

Merged

Conversation

andrewwdye
Copy link
Contributor

@andrewwdye andrewwdye commented Nov 18, 2024

Why are the changes needed?

Our use of a BucketRateLimiter with DelayingQueue is problematic when repeatedly inserting the same items. The former continually moves out the rate limit token time with each new item (no deduping), while the latter dedupes insertions. The net effect is that under load newly inserted items end up with a token time way in the future while the queue happily drains to empty with items processed at a much lower than expected rate.

What changes were proposed in this pull request?

This PR adds a new DedupingBucketRateLimiter that maintains historic reservations per item and skips taking out a new reservation when a duplicate item is found with an outstanding reservation. Reservations are dropped from the map when Forget is called, which happens after the item is fully processed. This is similar to how the exponential backoff rate limiter works. See here.

How was this patch tested?

Ran new rate limiter under high execution load and verify that queue is kept full but not overflown.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Sorry, something went wrong.

@andrewwdye andrewwdye requested a review from hamersaw November 18, 2024 19:10
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 13.31967% with 423 lines in your changes missing coverage. Please review.

Project coverage is 37.05%. Comparing base (8e9616a) to head (224c732).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
flytepropeller/pkg/controller/mocks/limiter.go 6.56% 296 Missing and 3 partials ⚠️
flytepropeller/pkg/controller/mocks/reservation.go 17.69% 90 Missing and 3 partials ⚠️
flytepropeller/pkg/controller/rate_limiter.go 42.30% 30 Missing ⚠️
flytepropeller/pkg/controller/workqueue.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6025      +/-   ##
==========================================
- Coverage   37.14%   37.05%   -0.09%     
==========================================
  Files        1313     1316       +3     
  Lines      131619   132100     +481     
==========================================
+ Hits        48891    48951      +60     
- Misses      78481    78896     +415     
- Partials     4247     4253       +6     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.10% <ø> (ø)
unittests-flytecopilot 22.23% <ø> (ø)
unittests-flytectl 62.46% <ø> (ø)
unittests-flyteidl 7.25% <ø> (ø)
unittests-flyteplugins 53.68% <ø> (ø)
unittests-flytepropeller 42.63% <13.31%> (-0.49%) ⬇️
unittests-flytestdlib 57.57% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Our use of a `BucketRateLimiter` with `DelayingQueue` is problematic when repeatedly inserting the same items. The former continually moves out the rate limit token time with each new item (no deduping), while the latter dedupes insertions. The net effect is that under load newly inserted items end up with a token time way in the future while the queue happily drains to empty with items processed at a much lower than expected rate.

This PR adds a new `DedupingBucketRateLimiter` that maintains historic reservations per item and skips taking out a new reservation when a duplicate item is found with an outstanding reservation. Reservations are dropped from the map when `Forget` is called, which happens after the item is fully processed. This is similar to how the exponential backoff rate limiter works. See [here](https://github.com/kubernetes/client-go/blob/master/util/workqueue/default_rate_limiters.go#L82-L149).

- [x] Run new rate limiter under high execution load and verify that queue is kept full but not overflown.

Signed-off-by: Andrew Dye <[email protected]>
@andrewwdye andrewwdye force-pushed the union/upstream-2f18ffe8f26e9c1e9a403945e6746225ef2fc1e4 branch from b3dd9d3 to 224c732 Compare November 18, 2024 20:17
@andrewwdye andrewwdye merged commit ed87fa1 into master Nov 18, 2024
50 of 52 checks passed
@andrewwdye andrewwdye deleted the union/upstream-2f18ffe8f26e9c1e9a403945e6746225ef2fc1e4 branch November 18, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants